Refactor build pipeline to reduce duplicate builds #778
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the build pipeline to centralize software version management and eliminate duplicate builds by deduplicating dependency software across images. The refactored workflow builds each dependency once and then builds multiple quickstart images, while adding support for scheduled builds, workflow calls from external repositories, and stable branch reference handling.
Key changes include:
- Centralized image and dependency configuration through
images.json - Unified build workflow that deduplicates dependency builds across all images
- Enhanced action to support artifact-based image loading for custom builds
Reviewed Changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| images.json | Centralized configuration defining all quickstart image variants with their dependencies |
| .github/workflows/ci.yml | New unified CI workflow replacing multiple separate build workflows |
| .github/workflows/build.yml | Refactored build workflow supporting custom image configurations and dependency deduplication |
| .github/workflows/push.yml | New workflow handling image pushing with manifest creation |
| action.yml | Enhanced action supporting artifact-based image loading for custom builds |
| Makefile | Updated to use centralized configuration from images.json |
| start | Enhanced version reporting to include all software components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@leighmcculloch This is a hard one to review. I asked Claude to give a pass on this. Here's what it said. Maybe it's worth addressing some of the findings? 🐛 Issues Found1. CRITICAL: Type Mismatch in action.ymlFile: artifact:
description: "Artifact to collect image from"
type: boolean
default: falseProblem: The name: ${{ inputs.artifact }} # Expects artifact name like "image-quickstart-custom-amd64.tar"Impact: This breaks the documented usage in README.md where users pass artifact: image-quickstart-custom-amd64.tarFix Required: artifact:
description: "Artifact to collect image from"
type: string
default: ""2. Unclear Logic: inputs.image_json ReferenceFile: enable:
${{ inputs.image_json && fromJSON('["core,rpc,horizon"]') ||
fromJSON('["core","rpc","core,rpc,horizon"]') }}Problem: References Likely Intent: Should be checking if this is a custom build (workflow_call)
Suggested Fix: enable:
${{ github.event_name == 'workflow_call' && fromJSON('["core,rpc,horizon"]')
|| fromJSON('["core","rpc","core,rpc,horizon"]') }}3. Path Inconsistency: image.json vs /image.jsonFile: echo " $(< image.json jq -r '.deps[] | select(.name == "friendbot") | "\(.ref) (\(.sha))"')"
echo " $(< image.json jq -r '.deps[] | select(.name == "lab") | "\(.ref) (\(.sha))"')"File: ADD .image.json /image.jsonProblem: The Dockerfile copies Current State: Works because start script runs from Suggested Fix: Be explicit in start script: echo " $(< /image.json jq -r '.deps[] | select(.name == "friendbot") | "\(.ref) (\(.sha))"')"
echo " $(< /image.json jq -r '.deps[] | select(.name == "lab") | "\(.ref) (\(.sha))"')"4. Inconsistent Job Numbering in ci.ymlFile: build:
name: 2 build
action-using-artifact:
name: 4 test action artifact # ← Jumps from 2 to 4
push:
name: 3 push # ← Goes back to 3
action-using-registry:
name: 4 test action registry # ← Another 4Problem: Job numbering is out of order. After "2 build", it jumps to "4 test Execution Order Analysis: Both "push" and "action-using-artifact" start after build completes (parallel), Suggested Fix: build:
name: 2 build
action-using-artifact:
name: 3 test action artifact
push:
name: 3 push
action-using-registry:
name: 4 test action registryOr if you want to show they run in parallel at the same level: build:
name: 2 build
action-using-artifact:
name: 3a test action artifact
push:
name: 3b push
action-using-registry:
name: 4 test action registry⚖️ Risk AssessmentLow Risk:
Medium Risk:
High Risk:
📝 RecommendationREQUEST CHANGES - Fix the critical issues before merge:
|
|
@fnando Thanks! Addressed the four issues. |
| # Process images.json through the images-with-extras script | ||
| IMAGE_JSON=.image.json | ||
| .image.json: images.json .scripts/images-with-extras | ||
| < images.json .scripts/images-with-extras | jq '.[] | select(.tag == "$(TAG)")' > $@ |
There was a problem hiding this comment.
should there be error handling for tag not found, I think the exit code from jq will be 0 if matches none and the build might continue and get weird errors later, if this could detect invalid TAG might help troubleshoot?
There was a problem hiding this comment.
wow, new build pipelines are slick, nice work. will start to look into integration of this on system-test/92
❤️ Thanks! I can pair with you on any of this if anything doesn't feel right or not sure, lmk. |
@leighmcculloch , I'm integrating this pipeline into stellar-rpc/e2e.yml, following your example integration on core, looking fairly good fit, I will reach out if things get stuck, thanks! |
|
@leighmcculloch , I've integrated this build pipeline into the stellar-rpc/e2e.yml which runs a new version of system-test that no longer builds or embeds quickstart: The updated system-test version has removed all references to quickstart. Tests can only be configured to run against a remote rpc url: starting testing on it now with these pr's and will fix as needed. One thing I'm wondering about is if we could add stellar cli as another configurable image to build in the pipeline(images.json)? it wouldn't get included in quickstart image, but downstream jobs liike stellar-rpc/e2e.yml, require the stellar cli and so that component is still built from source by system-test, but if an image of stellar-cli was available, system-test can be configured to just use that image and skip the build to reduce overall test times. |
|
Looking at your PRs, and looking at the tests I've run too, there's a general pattern that often we could run with whatever components quickstart latest or testing uses, and it might be helpful to define a way in the images.json to define that, such as an I'm thinking something like this: images: |
[
{
"inherits": "testing",
"tag": "rpc-custom",
"config": {
"protocol_version_default": 23
},
"deps": [
{ "name": "rpc", "repo": "${{ github.repository }}", "ref": "${{ github.ref }}" },
],
"additional-tests": []
}
] |
sounds good. it's intended mainly to derive just the
Please disregard that, I'm using image caching in this new system-test/test-workflow.yml callable which takes care of caching the cli image as part of the overall system-test image. using it in stellar-rpc/520 |
What
Refactor build pipeline to:
The new workflow also comes with some bonus features:
Old Workflow
flowchart TD subgraph Main Branch Workflow subgraph Latest Pipeline subgraph Latest AMD64 Deps Pipeline B1_latest_amd64[xdr] --> B_latest_amd64 B2_latest_amd64[core] --> B_latest_amd64 B3_latest_amd64[rpc] --> B_latest_amd64 B4_latest_amd64[horizon] --> B_latest_amd64 B5_latest_amd64[friendbot] --> B_latest_amd64 B6_latest_amd64[lab] --> B_latest_amd64 end B_latest_amd64[load/prepare/build deps amd64] --> C_latest_amd64[build latest quickstart amd64] C_latest_amd64 --> D_latest_amd64[test latest amd64] D_latest_amd64 --> E_latest_amd64[push latest amd64] E_latest_amd64 --> F_latest[push-manifest latest amd64+arm64] F_latest --> G_latest[action latest amd64+arm64] subgraph Latest ARM64 Deps Pipeline B1_latest_arm64[xdr] --> B_latest_arm64 B2_latest_arm64[core] --> B_latest_arm64 B3_latest_arm64[rpc] --> B_latest_arm64 B4_latest_arm64[horizon] --> B_latest_arm64 B5_latest_arm64[friendbot] --> B_latest_arm64 B6_latest_arm64[lab] --> B_latest_arm64 end B_latest_arm64[load/prepare/build deps arm64] --> C_latest_arm64[build latest quickstart arm64] C_latest_arm64 --> D_latest_arm64[test latest arm64] D_latest_arm64 --> E_latest_arm64[push latest arm64] E_latest_arm64 --> F_latest end subgraph Testing Pipeline subgraph Testing AMD64 Deps Pipeline B1_testing_amd64[xdr] --> B_testing_amd64 B2_testing_amd64[core] --> B_testing_amd64 B3_testing_amd64[rpc] --> B_testing_amd64 B4_testing_amd64[horizon] --> B_testing_amd64 B5_testing_amd64[friendbot] --> B_testing_amd64 B6_testing_amd64[lab] --> B_testing_amd64 end B_testing_amd64[load/prepare/build deps amd64] --> C_testing_amd64[build testing quickstart amd64] C_testing_amd64 --> D_testing_amd64[test testing amd64] D_testing_amd64 --> E_testing_amd64[push testing amd64] E_testing_amd64 --> F_testing[push-manifest testing amd64+arm64] F_testing --> G_testing[action testing amd64+arm64] subgraph Testing ARM64 Deps Pipeline B1_testing_arm64[xdr] --> B_testing_arm64 B2_testing_arm64[core] --> B_testing_arm64 B3_testing_arm64[rpc] --> B_testing_arm64 B4_testing_arm64[horizon] --> B_testing_arm64 B5_testing_arm64[friendbot] --> B_testing_arm64 B6_testing_arm64[lab] --> B_testing_arm64 end B_testing_arm64[load/prepare/build deps arm64] --> C_testing_arm64[build testing quickstart arm64] C_testing_arm64 --> D_testing_arm64[test testing arm64] D_testing_arm64 --> E_testing_arm64[push testing arm64] E_testing_arm64 --> F_testing end subgraph Future Pipeline subgraph Future AMD64 Deps Pipeline B1_future_amd64[xdr] --> B_future_amd64 B2_future_amd64[core] --> B_future_amd64 B3_future_amd64[rpc] --> B_future_amd64 B4_future_amd64[horizon] --> B_future_amd64 B5_future_amd64[friendbot] --> B_future_amd64 B6_future_amd64[lab] --> B_future_amd64 end B_future_amd64[load/prepare/build deps amd64] --> C_future_amd64[build future quickstart amd64] C_future_amd64 --> D_future_amd64[test future amd64] D_future_amd64 --> E_future_amd64[push future amd64] E_future_amd64 --> F_future[push-manifest future amd64+arm64] F_future --> G_future[action future amd64+arm64] subgraph Future ARM64 Deps Pipeline B1_future_arm64[xdr] --> B_future_arm64 B2_future_arm64[core] --> B_future_arm64 B3_future_arm64[rpc] --> B_future_arm64 B4_future_arm64[horizon] --> B_future_arm64 B5_future_arm64[friendbot] --> B_future_arm64 B6_future_arm64[lab] --> B_future_arm64 end B_future_arm64[load/prepare/build deps arm64] --> C_future_arm64[build future quickstart arm64] C_future_arm64 --> D_future_arm64[test future arm64] D_future_arm64 --> E_future_arm64[push future arm64] E_future_arm64 --> F_future end G_latest --> H[complete] G_testing --> H G_future --> H endNew Workflow
flowchart TD subgraph New Workflow I[setup] subgraph Deps Pipeline I --> J1[xdr amd64] I --> J2[xdr arm64] I --> J3[core amd64] I --> J4[core arm64] I --> J5[rpc amd64] I --> J6[rpc arm64] I --> J7[horizon amd64] I --> J8[horizon arm64] I --> J9[friendbot amd64] I --> J10[friendbot arm64] I --> J11[lab amd64] I --> J12[lab arm64] J1 --> R J2 --> R J3 --> R J4 --> R J5 --> R J6 --> R J7 --> R J8 --> R J9 --> R J10 --> R J11 --> R J12 --> R end R[load/prepare/build deps] subgraph Latest Pipeline R --> L_latest_amd64[build latest quickstart amd64] R --> L_latest_arm64[build latest quickstart arm64] L_latest_amd64 --> M_latest_amd64[test latest amd64] L_latest_arm64 --> M_latest_arm64[test latest arm64] M_latest_amd64 --> N_latest_amd64[push latest amd64] M_latest_arm64 --> N_latest_arm64[push latest arm64] N_latest_amd64 --> O_latest[push-manifest latest amd64+arm64] N_latest_arm64 --> O_latest O_latest --> P_latest[action latest amd64+arm64] end subgraph Testing Pipeline R --> L_testing_amd64[build testing quickstart amd64] R --> L_testing_arm64[build testing quickstart arm64] L_testing_amd64 --> M_testing_amd64[test testing amd64] L_testing_arm64 --> M_testing_arm64[test testing arm64] M_testing_amd64 --> N_testing_amd64[push testing amd64] M_testing_arm64 --> N_testing_arm64[push testing arm64] N_testing_amd64 --> O_testing[push-manifest testing amd64+arm64] N_testing_arm64 --> O_testing O_testing --> P_testing[action testing amd64+arm64] end subgraph Future Pipeline R --> L_future_amd64[build future quickstart amd64] R --> L_future_arm64[build future quickstart arm64] L_future_amd64 --> M_future_amd64[test future amd64] L_future_arm64 --> M_future_arm64[test future arm64] M_future_amd64 --> N_future_amd64[push future amd64] M_future_arm64 --> N_future_arm64[push future arm64] N_future_amd64 --> O_future[push-manifest future amd64+arm64] N_future_arm64 --> O_future O_future --> P_future[action future amd64+arm64] end P_latest --> Q[complete] P_testing --> Q P_future --> Q endUsage Building Custom Quickstarts
The new workflow has its build workflow separated from the push workflow so that other repos can call into the build workflow to build a custom quickstart for use in their own tests.
To run a quickstart that exists in DockerHub, do as you can do today:
To run a quickstart that has been built with a custom configuration, do this:
See an example of that working here:
And example of using it with the stellar/stellar-core repo:
Why
This work started out to make some small improvements to the build pipeline, because the pipeline defined versions in multiple places and people were understandably frequently forgetting to update the versions everywhere they needed to be.
The work then extended to improve the scalability to reduce the time spend rebuilding the same software in parallel when the same software was used for multiple images during the same build.
During the work it occurred to me that if structured appropriately the rewrite could present the pipeline in a way to also support nightly scheduled runs and even building from other repos.
The bonus features were included because it's always been an annoying shortcoming of quickstart that it can build anything, but only for itself. And it would be helpful if you could use quickstarts pipeline to build nightly builds, or any other custom builds of all the software that ends up in quickstart.
In the past we've built secondary pipelines in things like system-test, rather than leveraging the caching and build process that exists here in quickstart. With this change, repos like system-test will be able to stay focused on their test cases, and less on trying to replicate the build process.
Close #754
Close #651
Close #705
cc @stellar/devx @stellar/platform-eng @sisuresh